-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [#449] Implement HTTP response stream #92
Conversation
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
context_response_test.go
Outdated
scanner := bufio.NewScanner(resp.Body) | ||
var output []string | ||
for scanner.Scan() { | ||
output = append(output, scanner.Text()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt that this way can't test the stream accurately. I asked GPT, could you try the code below?
package main
import (
"time"
"github.com/gofiber/fiber/v2"
)
func streamData(c *fiber.Ctx) error {
c.Set(fiber.HeaderContentType, fiber.MIMETextHTMLCharsetUTF8)
streamErr := c.Context().SetBodyStreamWriter(func(w *fiber.StreamWriter) error {
for i := 0; i < 10; i++ {
_, err := w.Write([]byte("Streaming data " + string(i+48) + "..."))
if err != nil {
return err
}
time.Sleep(100 * time.Millisecond) // Simulate delay between chunks
}
return nil
})
if streamErr != nil {
return streamErr
}
return nil
}
func main() {
app := fiber.New()
app.Get("/stream", streamData)
app.Listen(":3000")
}
package main
import (
"bufio"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/gofiber/fiber/v2"
"github.com/stretchr/testify/assert"
)
func TestStreamData(t *testing.T) {
app := fiber.New()
app.Get("/stream", streamData)
req := httptest.NewRequest("GET", "/stream", nil)
resp, err := app.Test(req, -1) // -1 means read until we get the full response or the connection is closed
assert.NoError(t, err)
assert.Equal(t, 200, resp.StatusCode)
assert.Equal(t, resp.Header.Get("Content-Type"), fiber.MIMETextHTMLCharsetUTF8)
expectedChunks := []string{
"Streaming data 0...",
"Streaming data 1...",
"Streaming data 2...",
"Streaming data 3...",
"Streaming data 4...",
"Streaming data 5...",
"Streaming data 6...",
"Streaming data 7...",
"Streaming data 8...",
"Streaming data 9...",
}
reader := bufio.NewReader(resp.Body)
for i, expected := range expectedChunks {
chunk, err := reader.ReadString(' ')
if err != nil {
t.Fatalf("Failed to read chunk: %v", err)
}
chunk += ".." // Append rest of the string
rest, _ := reader.ReadString('.')
chunk += rest
assert.Equal(t, expected, strings.TrimSuffix(chunk, "\n"), "Mismatch in chunk %d", i+1)
// To simulate reading it with an interval as in real-time
time.Sleep(100 * time.Millisecond)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are similar, current test is also doing same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested them, and yes, both of them get the final response(a,b,c), then scan the result, this way can't test the output data verbatim. It's not the perfect solution, but I think it's good enough for us now. Great job.
f1cf120
context_response_test.go
Outdated
ConfigFacade = mockConfig | ||
} | ||
|
||
t.Run("Stream", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we don't need this line and beforeEach
in this case anymore, you can write the single test case directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR π
π Description
Closes goravel/goravel#449
@coderabbitai summary
β Checks